I'd say: a format string used to print the timestamp into the audit log messages. The format used is the same as strftime. Default is empty, meaning use the existing hard-coded format.

Done

just do "else" here. Easier to read.

Done

256 bytes in the stack ... this is a bit much. I'd allocate a buffer outside of the lock and pass it down. Or, better, it's going at the start of message anyway, can't you just print it there directly instead of using a temp buffer and then memcpy() into it?

Yeah, so here's what I changed. Removed the temp buffer, and directly stored the output of strftime in message buffer at the beginning, then used memmove to shift the output from the start to the end of reserved space.

don't do white-space only changes please!

Done

no white-space only changes please

Done

I'd use some named const size_t for the 1k constant

Done

why do you need to do this? Can't you just pass the actual length as you used to do? And then do the calculation inside this func?

Previously message was a fixed size array (1024). In the new version, its a pointer to an offset within a large buffer, so we must explicitly pass the correct size (MAX_AUDIT_PAYLOAD_LENGTH).

same here: use a named constant for 1k

Done

validity checks must be done in the check callback, not in the update callbac

Done.

Small issue: don't make it "empty = hard-coded", it's not very intuitive, simply set the default value of timestamp_format to the correct (backward compatible) value.

Big issue: don't use strftime. The server already has date-to-string formatting function DATE_FORMAT(). We should not let different parts of the server to implement formatting differently. For example, %M should always and everywhere have the same meaning.

That is, invoke server's date-to-string function. And to do that, it must be in a service. But, perhaps, you can add a new method to the timezone service and not create a new one.

I have updated the implementation and added a new method in the timezone service.

you can try set global server_audit_timestamp_format=default instead

Done

you can now remove references to the "system strftime" from comments

Done

let's drop these #ifndef/#endif, simply define what you want. Easier to read and we don't really expect anyone to define this outside of the plugin

Done

I thought it'd be natural to generate the timestamp string here and print like

      "%s,%.*s,%.*s,%.*s%s,%d,%lld,%s", ts_start, ...

Why did you move it to write_log() ?

I initially had the implmentation in log_header(). However I had to take a lock there for the timestamp buffer. So, overall it took 2 locks for an audit log. Moving it to write_log(), we will have to take just a single lock (existing one). That would be faster. Quoting gkodinov's comments here:

On your "taking the lock" question.

I'm not very familiar with with the code, but I'll take a stab:

What audit does (from what I can see) is:

char message[...];
prepare_the_message_as_much_as_possible(message);
rd_lock();
<the variable protected with the rw lock>->write_log(message);
release_rd_lock();

The above is done to decrease the time needed to hold on the lock.

Note that this lock is later expanded to also cover all of the config values (in addition to the logger_service variable). Not sure it's a great idea, but I'm not the one to judge.

And now you're adding a need to take the lock again during prepare.

Taking a read lock is usually considered expensive afair. So taking the lock twice is definitely not a good idea IMHO. One option would be to move the strftime() call inside the existing lock boundaries. You know the max length already. So you can probably prepare a buffer for it and concat the two just before sending. This will be a good idea if strftime does take longer and locks more compared to strcpy() and the cost of extra memory.

The other option is to extend the scope of the read lock to cover preparation too. This is easier and the effect is probably less, since the only important write lock acquisitions are during the SET for the plugin variables.

I'd personally advise towards trying the former for like a day or two. But please make sure not to call any allocs while doing that. This will have an even more devastating effect on performance than the lock extension. If it proves too hard I'd go with the latter.

well, the goal of the old code was to avoid multiple calls of the expensive get_timezone_information().
I think we can keep this logic, it doesn't complicate the code much

doesn't seem to be fixed?

I misunderstood that the current changes are fine. I have now made the necessary changes.

dunno, looks hackish. let's not add this comma in write_log, not
in log_header. then you won't need to remove it here

Okay, I have removed the comma from log_header(), and adding it now in write_log().

remove the space

Done

doesn't seem to be fixed?